Nan2 #72

sonnyp · 2015-09-21T15:19:36Z

WIP; could use some help

sonnyp · 2015-09-21T15:31:23Z

node-stringprep.cc


-    target->Set(NanNew<String>("StringPrep"), t->GetFunction());
+    target->Set(Nan::New<String>("StringPrep").ToLocalChecked(), t->GetFunction());


is this correct?

correct but you should use Nan::Set(target, Nan::New<String>("StringPrep").ToLocalChecked(), t->GetFunction()); now for max-compatibility

austinkelleher · 2015-09-23T22:17:05Z

+1 Need this very much. Looking to moving to Node 4.1.0.

philidem · 2015-09-24T18:31:13Z

👍

erulabs · 2015-09-24T23:16:59Z

👍 as well! Thank you for your efforts!

lloydwatkin · 2015-09-25T08:00:37Z

@rvagg is usually very helpful with nan stuff if we ask nicely... please? :)

erulabs · 2015-09-28T07:35:46Z

@sonnyp I've started another effort here: #74

austinkelleher · 2015-09-28T12:53:56Z

The Nan2 support that @erulabs has started is looking pretty good so far.

rvagg · 2015-09-29T13:36:38Z

node-stringprep.cc

@@ -43,11 +43,11 @@ class StringPrep : public ObjectWrap {

  static NAN_METHOD(New)
  {
-    NanScope();
+    Nan::HandleScope scope;


can be removed from inside all NAN_*() methods

lloydwatkin · 2015-09-29T14:09:49Z

You rule @rvagg, thanks!

sonnyp · 2015-09-29T14:59:37Z

node-stringprep.cc

-    NanScope();
-    Local<FunctionTemplate> t = NanNew<FunctionTemplate>(New);
+    Nan::HandleScope scope;
+    Local<FunctionTemplate> t = Nan::New<FunctionTemplate>(New);
    NanAssignPersistent(stringprep_constructor, t);


../node-stringprep.cc: In static member function ‘static void StringPrep::Initialize(v8::Handle<v8::Object>)’: ../node-stringprep.cc:24:50: error: ‘NanAssignPersistent’ was not declared in this scope NanAssignPersistent(stringprep_constructor, t);

stringprep_constructor needs to be declared as a Nan::Persistent now (instead of just Persistent, i.e. v8::Persistent) and then you use stringprep_constructor.Reset(t) here

sonnyp · 2015-09-29T15:44:29Z

node-stringprep.cc

  {
+    Nan::EscapableHandleScope scope
    size_t destLen = str.length() + 1;


../node-stringprep.cc: In member function ‘v8::Local<v8::Value> StringPrep::prepare(v8::String::Value&)’: ../node-stringprep.cc:115:5: error: expected initializer before ‘size_t’ size_t destLen = str.length() + 1;

sonnyp · 2015-09-29T15:57:27Z

@rvagg thanks a lot for the help

I ported @erulabs work from #74 and your comments here.

I added the last build errors as comments. Additionally if you could double check the PR that would be great.

erulabs · 2015-09-29T19:25:30Z

Hey @sonnyp looks like a missing semicolon on line #114

The remaining issue seems to be how we return the value (lines 228 & 279):

Local result = Nan::New(dest, destLen).ToLocalChecked();
info.GetReturnValue().Set(result);

This passes tests, but appears to break the unicode/ascii conversion. I tried a handful of ideas without much results. +1 to big big thanks to @rvagg 💃

sonnyp · 2015-09-30T09:01:31Z

@erulabs

../node-stringprep.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE ToUnicode(Nan::NAN_METHOD_ARGS_TYPE)’:

../node-stringprep.cc:228:11: error: missing template arguments before ‘result’

     Local result = Nan::New(dest, destLen).ToLocalChecked();

           ^

../node-stringprep.cc:228:11: error: expected ‘;’ before ‘result’

../node-stringprep.cc:230:31: error: ‘result’ was not declared in this scope

     info.GetReturnValue().Set(result);

https://github.com/node-xmpp/node-stringprep/pull/72/files#diff-4e17e0441c265ec99c496bc09f563bc2R228

erulabs · 2015-09-30T18:38:26Z

Yeah - it should be a Local for sure, which builds properly, but breaks the actual tests. I get the feeling that something here is breaking our unicode/ascii converstion

erulabs · 2015-10-05T18:14:33Z

I wonder if we can reach out to the original authors of node-stringprep.cc and see if they have some advice for us since it seems like we've hit a dead end :(
@pixelglow @dodo @lloydwatkin I hope this isn't bothersome - any pointers to help us convert to NAN2 without breaking the package would be hugely appreciated :) Thanks!

lloydwatkin · 2015-10-05T19:47:19Z

Actually @astro is the original creator AFAIK, I've just been keeping it ticking over. I'm afraid native module binding isn't something I'm familiar with.

sonnyp · 2015-10-06T08:57:27Z

As far as node-xmpp is concerned I don't think we should spend much time on node-stringprep. RFC 7622 deprecates the use of stringprep see https://github.com/node-xmpp/JID/issues/1

I have made a standalone and stringprep-free module for JID at https://github.com/node-xmpp/JID and plan to use it in node-xmpp-core see https://github.com/node-xmpp/node-xmpp-core/pull/76 . (Somehow all original tests passes without stringprep)

I understand it's not fully compliant but neither is node-xmpp-core JID anyway ( + other issues such as memory leaks and native module burden).

I plan to work on RFC 7622 compliance next.

ChALkeR · 2016-04-21T04:40:20Z

Tracking: nodejs/node#2798

astro · 2016-05-06T23:45:17Z

Fixed by #79

sonnyp reviewed Sep 21, 2015
View reviewed changes

sonnyp mentioned this pull request Sep 21, 2015

Fails for io.js 3.2.0 #70

Closed

rvagg reviewed Sep 29, 2015
View reviewed changes

sonnyp reviewed Sep 29, 2015
View reviewed changes

sonnyp mentioned this pull request Sep 29, 2015

node 4x retry #74

Closed

sonnyp reviewed Sep 29, 2015
View reviewed changes

nan2

ce83ceb

sonnyp force-pushed the nan2 branch from e05a5c0 to ce83ceb Compare September 29, 2015 15:59

sonnyp added 2 commits September 29, 2015 18:04

init

9f9b3d2

f

1a3bbf1

sonnyp added 2 commits September 30, 2015 10:55

f

e199f5a

f

faffe6b

mathewpeterson mentioned this pull request Oct 6, 2015

Issues with node-stringprep hipchat/hubot-hipchat#258

Open

sonnyp mentioned this pull request Oct 22, 2015

No longer builds #75

Open

sonnyp mentioned this pull request Nov 3, 2015

vulnerable: upgrade debug dependency #77

Merged

astro closed this May 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nan2 #72

Nan2 #72

sonnyp commented Sep 21, 2015

sonnyp Sep 21, 2015

rvagg Sep 29, 2015

austinkelleher commented Sep 23, 2015

philidem commented Sep 24, 2015

erulabs commented Sep 24, 2015

lloydwatkin commented Sep 25, 2015

erulabs commented Sep 28, 2015

austinkelleher commented Sep 28, 2015

rvagg Sep 29, 2015

lloydwatkin commented Sep 29, 2015

sonnyp Sep 29, 2015

rvagg Sep 29, 2015

sonnyp Sep 29, 2015

sonnyp commented Sep 29, 2015

erulabs commented Sep 29, 2015

sonnyp commented Sep 30, 2015

erulabs commented Sep 30, 2015

erulabs commented Oct 5, 2015

lloydwatkin commented Oct 5, 2015

sonnyp commented Oct 6, 2015

ChALkeR commented Apr 21, 2016

astro commented May 6, 2016


		target->Set(NanNew<String>("StringPrep"), t->GetFunction());
		target->Set(Nan::New<String>("StringPrep").ToLocalChecked(), t->GetFunction());

Nan2 #72

Nan2 #72

Conversation

sonnyp commented Sep 21, 2015

sonnyp Sep 21, 2015

Choose a reason for hiding this comment

rvagg Sep 29, 2015

Choose a reason for hiding this comment

austinkelleher commented Sep 23, 2015

philidem commented Sep 24, 2015

erulabs commented Sep 24, 2015

lloydwatkin commented Sep 25, 2015

erulabs commented Sep 28, 2015

austinkelleher commented Sep 28, 2015

rvagg Sep 29, 2015

Choose a reason for hiding this comment

lloydwatkin commented Sep 29, 2015

sonnyp Sep 29, 2015

Choose a reason for hiding this comment

rvagg Sep 29, 2015

Choose a reason for hiding this comment

sonnyp Sep 29, 2015

Choose a reason for hiding this comment

sonnyp commented Sep 29, 2015

erulabs commented Sep 29, 2015

sonnyp commented Sep 30, 2015

erulabs commented Sep 30, 2015

erulabs commented Oct 5, 2015

lloydwatkin commented Oct 5, 2015

sonnyp commented Oct 6, 2015

ChALkeR commented Apr 21, 2016

astro commented May 6, 2016